feat(windows): honour Windows "metered connection" flag for downloading updates#16099
feat(windows): honour Windows "metered connection" flag for downloading updates#16099rc-swag wants to merge 11 commits into
Conversation
This adds a helper unit which uses the windows API to obtain a ConnectionProfile. This profile provides information about the connection status and connectivity statistics. This includes the connection costs, roaming, restricted etc. It also can check to see if background networking activity has been restricted. Using the recommened example this commit combines these to determine if a network is metered. It also provides access to the background network being restricted. Fixes: #13566
User Test ResultsTest specification and instructions
Results TemplateTest Artifacts |
If the download has already occured before calling the pop up then there is no reason to warn about the metered connection.
Co-authored-by: Eberhard Beilharz <ermshiperete@users.noreply.github.com>
| if HasKeymanRun OR IsMetered then | ||
| begin | ||
| frmStartInstallNow := TfrmStartInstall.Create(nil, true); | ||
| frmStartInstallNow := TfrmStartInstall.Create(nil, HasKeymanRun, IsMetered); |
There was a problem hiding this comment.
From devin.ai:
Contradictory dialog messages when IsMetered=True and HasKeymanRun=False in Update_ApplyNow
In UfrmMain.pas:831, when HasKeymanRun is False but IsMetered is True, the form is created with RestartRequired=False and ReadyToInstall=False (default). This causes FormCreate at Keyman.Configuration.UI.UfrmStartInstall.pas:72-73 to show S_Ready_To_Install ("An update to Keyman has been downloaded and is ready to install") while simultaneously displaying the metered warning S_Metered_Warning ("You're on a metered connection. Downloading now may incur data charges") at line 77. These messages directly contradict each other: one says the update has already been downloaded, while the other warns about downloading costs. Before this PR, this code path didn't exist — the old code only showed the dialog when HasKeymanRun was true, always passing RestartRequired=true, so S_Ready_To_Install was never displayed here.
Co-authored-by: Eberhard Beilharz <ermshiperete@users.noreply.github.com>
Keyman currenlty only supports Windows 10 and 11 so the exception was low risk. If it is not available we return false as metered connections werent possilble before this the functionality will be the consistent with those windows versions. i.e. always updating in the background
|
@Meng-Heng We need to test some scenarios again after my changes thanks. |
mcdurdin
left a comment
There was a problem hiding this comment.
This change looks really solid.
One thing: I am not that comfortable with the UI of the dialog -- the red background is not consistent with styling of the rest of the app and I think we should use a standard Warning icon instead. Happy for that to be a follow-up PR.
There was a problem hiding this comment.
Can we use the modern file naming scheme:
Keyman.Configuration.Util.NetworkConnection.pas?
| Profile: IConnectionProfile; | ||
| CostLevel: IConnectionCost; |
There was a problem hiding this comment.
Do we need to initialize COM before these functions are called? If so, we should call that out in the docs
There was a problem hiding this comment.
I'm not sure how to determine this.
There was a problem hiding this comment.
https://blogs.embarcadero.com/a-tale-of-3-apis-vcl-integration-with-winapi-com-shellapi-winrt/
This is the first use of WinRT in the Keyman code. We should be moving towards it across the codebase. AFAICT, yes, we should initialize COM (which we already do) before calling these functions, but: https://microsoft.github.io/MIDI/kb/threading-and-initialization/ more to learn!
There was a problem hiding this comment.
| // If the WinRT network APIs are unavailable or throw (e.g. unusual | ||
| // Windows SKUs, containers, Network List Manager service stopped), | ||
| // treat the connection as non-metered so updates are not blocked. | ||
| Result := False; |
There was a problem hiding this comment.
Can we report errors on Sentry please? (Silent swallowing of the error is okay)
| except | ||
| on E: Exception do | ||
| // See IsMetered: default to not-restricted if the WinRT APIs throw. | ||
| Result := False; |
| icRestartRequiredMetered, | ||
| icRestartRequiredNotMetered, | ||
| icReadyToInstallNotMetered, // Metered warning never needed if ReadyToInstall | ||
| icNoInstallMessageMetered |
There was a problem hiding this comment.
I found this install case name confusing; is the following name accurate?
| icNoInstallMessageMetered | |
| icReadyToDownloadWarnMetered |
I guess my confusion extends to the dialog box as well -- would it be possible for the dialog box to say something to clarify that a download is required, something like this?
The update has not yet been downloaded.
and
You're on a metered connection. Downloading now may incur data charges.
There was a problem hiding this comment.
I don't like the name change suggested because it overlaps with the first case. I can see the confusion though with you suggested messages.
In thinking about the fact I have not got on to this issue # #13711 yet. The forground update is still confusing to the user especially if they are on a slow connection and click Check for Updates followed by Install Now on a slow connection it there will be no gui while it is downloading the update. Untill #13711 we can provide more information (at least in english, hopefully it gets translated will in the pop-up) I am worried though it maybe to much and confusing.
Scenarios.
Foreground Update Tab:
Messages that can be displayed in isolation or combination off.
- Installing the update now will require a restar of your computer before you can use Keyman again. Update now anyway
- The Update has not yet been downloaded
Keyman Configuration will close and it will be downloaded in the background before installing
[The second part of this message in `` will be untill feat(windows): apply update now give progress if download still in progress #13711 is implemented then removed.] - You're on a metered connection. Downloading now may incur data charges.
OnBoot:
icReadyToInstallNotMetered
This is the true backroung update scenario. The update has downloaded in the background the user is starting Keyman, it has not been run we are just letting them know we are about to install the new version and giving them an option to cancel
- An update to Keyman has been downloaded and is ready to install.
Now this code can be improved to actually use the StateMachine to check the state. It could be downloading or in updateavailable (if automatic updates are off).
Then for Update Tab
DownloadRequired && RestartRequired && Metered
-> icRestartRequiredMetered= show 1-3
DownloadRequired && RestartRequired && !Metered
-> icRestartRequiredNotMetered = show 1-2
DownloadRequired && !RestartRequired && !Metered
-> icDownloadRequired = show 2 (this is really now just informing about the pending silence :))
DownloadRequired && !RestartRequired && Metered
-> icNoInstallMessageMetered = show 2 & 3
!DownloadRequired && !RestartRequired && !Metered
-> no pop up required
Then for boot up
icReadyToInstallNotMetered = show 4.
Below I is the same as above but have renamed the enums for clarity with the additonal download message.
DownloadRequired && RestartRequired && Metered
-> icDownloadRestartMetered= show 1-3
DownloadRequired && RestartRequired && !Metered
-> icDownloadRestart = show 1-2
DownloadRequired && !RestartRequired && !Metered
-> icDownload = show 2 (this is really now just informing about the pending silence :))
DownloadRequired && !RestartRequired && Metered
-> icDownloadMetered = show 2 & 3
!DownloadRequired && !RestartRequired && !Metered
-> no pop up required
Then for boot up
icReadyToInstall = show 4.
Noted with thanks, @rc-swag. I'll test this tomorrow. |
Co-authored-by: Marc Durdin <marc@durdin.net>
I will change the colour to yellow to match the No keyboards installed. a quick looks shows we currently don't have the yellow trianlge exclamantion mark icon in the repo. I might make that a quick follow up PR. |

This adds a helper unit which uses the windows API to obtain a ConnectionProfile. This profile provides information about the connection status and connectivity statistics. This includes the connection costs, roaming, restricted etc. It also can check to see if background networking activity has been restricted.
Using the recommened example this commit combines these to determine if a network is metered. It also provides access to the background network being restricted.
There is a call that
IsBackgroundUpdateAllowedthat will return true if a background update is allowed.This is then use in the Update State Machine and also by update pop up.
UpdateStateMachine
Before downloading
IsBackgroundUpdateAllowedis calledUpdate Pop Up
This uses the
IsMeteredcall and adds a warning if to the user who is wanting to Install Now.This message would be better as a banner in the Update Configuration tab, rather than a delphi tab. This is the most efficent place to add the message for now.
Fixes: #13566
Build-bot: release:windows
User Testing
Mark connection as metered
For this we need to use windows setting to set the test machines network to metered. Right click the network connection icon in the Windows System tray and select
network & internet settingsSelected the connected work and the change the toggle forMetered connectiontoOnWindows 11

Windwos 10

TEST_RESTARTREQ_METERED
Use a keyboard update to test.
TEST_RESTARTREQ_NOT_METERED
This is to make sure a metered warning message is not present on non-metered connection.
TEST_NO_RESTARTREQ_METERED
This is to make sure a metered warning message is present but no other messages.
TEST_NO_READY_TO_INSTALL_NOT_METERED
This is to make sure a metered connection message is not shown when
we already have the download and are just in the WaitingRestart State.
TEST_NO_BACKGROUND_UPDATE
regeditupdate stateback tousIdlelast update check time. This needs to be done otherwise it will stay inusIdlefor 7 days.c:\Program Files (x86)\Keyman\Keyman Desktopkmshell.exe -cThis will run the configurationTEST_BACKGROUND_UPDATE
This is a regresion test.
OFFas explanined above.regeditupdate stateback tousIdlelast update check time. This needs to be done otherwise it will stay inusIdlefor 7 days.c:\Program Files (x86)\Keyman\Keyman Desktopkmshell.exe -cThis will run the configuration